Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue where users would get locked out when using OTP tokens. #1383

Merged
merged 2 commits into from
Oct 11, 2017

Conversation

russjones
Copy link
Contributor

Purpose

For local accounts to obtain SSH certificates, connect to an AuthTunnel with the AuthMethod being password, password+otp, or password+u2f. When we added WithUserLock in #1306 we didn't take into account that the Go HTTP client would use a connection pool to make requests. Since the OTP token can only be used once and the HTTP client would open multiple connections, this causes the fail count to go up by 2 for every successful login. In addition, successful logins would not decrease the fail count.

This means after two successful login with password+otp, you would be locked out of Teleport.

Implementation

The long term fix for this is to separate out obtain a SSH certificate from operations on the Auth Server. The short term fix is as follows.

  1. Limit the TunClient to a single request. Added the /ca/user/certs/bundle endpoint and added TunDisableRefresh to enable this.
  2. Upon successful login wipe out failed counter.
  3. Shorten the TTL for failed attempts to expire.

Related Issues

Fixes #1347

@russjones russjones force-pushed the rjones/real-fix-user-lock branch 2 times, most recently from 458bb73 to 4b4e036 Compare October 11, 2017 00:20
@russjones russjones force-pushed the rjones/real-fix-user-lock branch from de5af00 to 3d0ce56 Compare October 11, 2017 22:57
@russjones russjones merged commit 17631c4 into master Oct 11, 2017
@russjones russjones deleted the rjones/real-fix-user-lock branch October 11, 2017 23:00
hatched pushed a commit that referenced this pull request Dec 20, 2022
* tshd events: Wait for listeners before responding

When tshd is sending the relogin event, it needs to be able to know when
the Electron app has finished relogging the user. I wanted to implement
this by simply waiting for the response from the RPC.

I'm so glad we did not use gRPC streams for tshd events as this would be
much harder to implement with streams.

* Return a function from ModalsService.openDialog which closes dialog

With the introduction of important and regular modals, this will help us
close the specific dialog if need arises.

* Make tshd events listeners aware of request cancellation

This will be useful in the upcoming commits. Basically, tshd is going to
ask the Electron app to relogin the user, with a 1 minute timeout.
The Electron app will show a login modal but if the user doesn't submit
it within 1 minute, tshd is going to cancel the request.

In that situation, we need to be able to close the modal.

* Add support for passing reason in DialogClusterConnect

* Add support for important modals

This will let us show the relogin modal on expired cert, even if the user
was using some other modal at that moment.

* Remove title attr from notification text

The user can read more by expanding the notification. The title attribute
persisted even after expanding the notification, making reading it harder.

* Add WindowsManager.forceFocusWindow

* Use IAppContext instead of AppContext

The next commit is going to add a private method to AppContext.
IAppContext is an interface which enables us to pass a mocked version of
AppContext in tests. That mock is not going have that private method,
so any place accepting AppContext wouldn't be able to accept the mocked
AppContext.

Instead, classes & functions should accept IAppContext rather than AppContext.

* Implement handlers for new tshd events

tshd needs to be able to do two things:

- Ask the user to log in again.
- Forward errors from goroutines running gateways to the Electron app
  in form of a notification. Otherwise those error would be visible
  only in the logs.

* Don't restart gateways after logging in

Restarting the gateways on login was a workaround from times where gateways
didn't manage their own certs.

In the new flow, a gateway takes care of refreshing the certs itself
through the middleware passed to alpnproxy.LocalProxy.

syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two
functions:

- syncRootClusterAndCatchErrors
- restartClusterGatewaysAndCatchErrors

The second function is no longer necessary, so we can make any place that
was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors
call just syncRootClusterAndCatchErrors instead.
hatched pushed a commit that referenced this pull request Jan 30, 2023
…1416)

* tshd events: Wait for listeners before responding

When tshd is sending the relogin event, it needs to be able to know when
the Electron app has finished relogging the user. I wanted to implement
this by simply waiting for the response from the RPC.

I'm so glad we did not use gRPC streams for tshd events as this would be
much harder to implement with streams.

* Return a function from ModalsService.openDialog which closes dialog

With the introduction of important and regular modals, this will help us
close the specific dialog if need arises.

* Make tshd events listeners aware of request cancellation

This will be useful in the upcoming commits. Basically, tshd is going to
ask the Electron app to relogin the user, with a 1 minute timeout.
The Electron app will show a login modal but if the user doesn't submit
it within 1 minute, tshd is going to cancel the request.

In that situation, we need to be able to close the modal.

* Add support for passing reason in DialogClusterConnect

* Add support for important modals

This will let us show the relogin modal on expired cert, even if the user
was using some other modal at that moment.

* Remove title attr from notification text

The user can read more by expanding the notification. The title attribute
persisted even after expanding the notification, making reading it harder.

* Add WindowsManager.forceFocusWindow

* Use IAppContext instead of AppContext

The next commit is going to add a private method to AppContext.
IAppContext is an interface which enables us to pass a mocked version of
AppContext in tests. That mock is not going have that private method,
so any place accepting AppContext wouldn't be able to accept the mocked
AppContext.

Instead, classes & functions should accept IAppContext rather than AppContext.

* Implement handlers for new tshd events

tshd needs to be able to do two things:

- Ask the user to log in again.
- Forward errors from goroutines running gateways to the Electron app
  in form of a notification. Otherwise those error would be visible
  only in the logs.

* Don't restart gateways after logging in

Restarting the gateways on login was a workaround from times where gateways
didn't manage their own certs.

In the new flow, a gateway takes care of refreshing the certs itself
through the middleware passed to alpnproxy.LocalProxy.

syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two
functions:

- syncRootClusterAndCatchErrors
- restartClusterGatewaysAndCatchErrors

The second function is no longer necessary, so we can make any place that
was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors
call just syncRootClusterAndCatchErrors instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login locks the user for 15 minutes
2 participants